Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error if PAuth language features are used on unsupported CPU #66

Merged
merged 1 commit into from
Feb 12, 2024

Conversation

atrosinenko
Copy link
Contributor

@atrosinenko atrosinenko commented Dec 21, 2023

Check for several -fptrauth-* language features that require CPU with FEAT_PAuth support. If such unsupported feature is requested, make Clang frontend print error and exit early.

@atrosinenko atrosinenko self-assigned this Dec 21, 2023
@atrosinenko
Copy link
Contributor Author

Looks like HasPAuth and a few other flags should be conditionally set in AArch64TargetInfo::setArchFeatures function, but they don't: 9e3a02b Strictly speaking, these flags do not directly correspond to FEAT_* identifiers (example: HasCCPP), so this should be checked a bit more...

@atrosinenko
Copy link
Contributor Author

As far as I can see, it is Clang driver that sets the per-triple default CPU and features, but clang -cc1 enables several pauth-related features based on arm64e-* triple. Thus, I have a number of options:

  • enable pauth in driver instead, fix the affected cc1 tests by adding the new options enabling pointer authentication in clang -cc1 tests
  • explicitly pass -target-feature +pauth to clang -cc1, where needed, to make existing tests pass
  • emit error messages inside the driver, not frontend (the driver probably lacks available CPU features in parsed form)

I plan implementing the second variant (add target-feature / target-cpu to existing tests). This seems safer than the first option (less probability of accidentally disabling pointer authentication).

@asl
Copy link
Contributor

asl commented Dec 22, 2023

@atrosinenko I think everything should be done in driver. Tests are likely to be fixed.

@atrosinenko
Copy link
Contributor Author

As far as I can see, it is Clang driver that sets the per-triple default CPU and features, but clang -cc1 enables several pauth-related features based on arm64e-* triple.

Sorry, this seems to be incorrect: both choosing the default CPU/features and -fptrauth-... options takes place in the driver, as expected. The only change in ~70 tests is to insert -target-feature +pauth options in invocations like this:

// RUN: %clang_cc1 -target arm64e-apple-ios -fptrauth-calls ... -emit-llvm ...

(i.e. those enabling ptrauth and not involving actual code generation either in assembler or object file form). Several tests still have to be fixed.

Maybe it is better to check for unsupported options in the driver instead. This should eliminate clang::PointerAuthOptions::getAllUsedSchemas() but may require more complicated checking for presence of +pauth feature. I will check this tomorrow.

@atrosinenko atrosinenko changed the title [WIP] Error out early if missing PAuth support is required Error if PAuth language features are used on unsupported CPU Dec 27, 2023
@atrosinenko atrosinenko requested review from kovdan01 and asl December 27, 2023 13:30
@atrosinenko atrosinenko removed their assignment Dec 27, 2023
@atrosinenko atrosinenko marked this pull request as ready for review December 27, 2023 13:32
@atrosinenko
Copy link
Contributor Author

A few notes:

  • In this patch, -fptrauth-intrinsics is considered as requiring +pauth feature. Some ptrauth_* intrinsics can be implemented by only using HINT-encoded opcodes (at least, signing and authentication using IA and IB keys). I guess, this can be useful in libraries such as musl. When upstreaming, it may be better to replace the check for Opts.PointerAuthIntrinsics with per-intrinsic checks at some other places.
  • The error message is usually printed twice (when AArch64TargetInfo::adjust is called from CompilerInstance::createTarget and from CompilerInstance::createPreprocessor). Hopefully, this is not a big issue at now.
  • This PR contains a commit that sets HasPAuth flag in AArch64TargetInfo::setArchFeatures if v8.3+ or v9.0+ extensions are enabled. It is worth sending a more elaborate patch upstream (there are a few other seemingly forgotten flags)

clang/include/clang/Basic/DiagnosticDriverKinds.td Outdated Show resolved Hide resolved
clang/test/CodeGen/ptrauth-cpu-feature.c Outdated Show resolved Hide resolved
clang/lib/Basic/Targets/AArch64.cpp Outdated Show resolved Hide resolved
clang/test/CodeGen/ptrauth-cpu-feature.c Outdated Show resolved Hide resolved
clang/test/CodeGen/ptrauth-cpu-feature.c Outdated Show resolved Hide resolved
@atrosinenko
Copy link
Contributor Author

llvm/llvm-project#78027 is the mainline PR that should replace 24048e1 commit in this PR.

@atrosinenko
Copy link
Contributor Author

Addressed the comments, except for using a more generic triple in the test. A few weeks ago I saw a Discourse thread on "generic vs. precise triples" - obviously "the more triples tested the better", but I cannot remember whether that was an argument for using generic triples or for picking an arbitrary triple that is the most familiar for the particular developer... I guess that it is better to have as precise triple as possible (even though arbitrarily chosen at commit time) for better reproducibility. Links to coding standards and Discourse threads are welcome.

Note: after all review comments would be ultimately addressed, I would like to manually squash and rebase, so that the commit that sets HasPAuth flag remains a separate commit to be rolled back and then replaced by mainline one (see llvm/llvm-project#78027).

@atrosinenko atrosinenko requested a review from kovdan01 January 17, 2024 16:53
clang/lib/Basic/Targets/AArch64.cpp Outdated Show resolved Hide resolved
clang/include/clang/Basic/DiagnosticDriverKinds.td Outdated Show resolved Hide resolved
clang/test/CodeGen/ptrauth-cpu-feature.c Outdated Show resolved Hide resolved
@atrosinenko
Copy link
Contributor Author

Once again, addressed everything except target triples :) Thanks for the link - I will update the test after a bit more research.

@atrosinenko
Copy link
Contributor Author

Changed the cases that do not rely on features not being implicitly enabled to -target aarch64 (assuming no features are ever implicitly disabled due to target-specific defaults).

@atrosinenko atrosinenko requested a review from kovdan01 January 27, 2024 08:49
Copy link
Contributor

@kovdan01 kovdan01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Check for several -fptrauth-* language features that require CPU with
FEAT_PAuth support. If such unsupported feature is requested, make
Clang frontend print error and exit early.
@atrosinenko atrosinenko force-pushed the atrosinenko/reject-unsupported-opts branch from eb62a64 to 1f7f0c7 Compare February 12, 2024 11:33
@atrosinenko
Copy link
Contributor Author

Dropped the commit that sets HasPAuth is v8.3+ (replaced by #74), squashed and rebased.

@atrosinenko atrosinenko merged commit e5dc43f into elf-pauth Feb 12, 2024
1 check passed
@atrosinenko atrosinenko deleted the atrosinenko/reject-unsupported-opts branch February 12, 2024 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[PAuth][clang] Error on passing PAuth-specific options on non-PAuth target
3 participants